-
Notifications
You must be signed in to change notification settings - Fork 348
chore: State tracking in State object #1383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: forks/osaka
Are you sure you want to change the base?
Conversation
def generate_merkle_proof_requests(state) -> Tuple[List[Address], List[Tuple[Address, Bytes32]]]: | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left this here to show what the next addition may look like -- will delete
elif access_type in [STORAGE_READ, STORAGE_WRITE]: | ||
if address not in tracker.storage_accessed_keys: | ||
tracker.storage_accessed_keys[address] = set() | ||
if key is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be safer to also raise an error if key is None
?
For a STORAGE_READ
or STORAGE_WRITE
sounds like if this sitaution happens, it should be an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep thats true -- @fselmo , this or the previous one were PRs that I was trying to merge into the execution specs, just want to confirm again that you will be adding the same functionality with your BAL PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep thats true -- @fselmo , this or the previous one were PRs that I was trying to merge into the execution specs, just want to confirm again that you will be adding the same functionality with your BAL PR?
Thanks for the ping! I brought this up recently in the STEEL meeting. I've mostly been coordinating that PR with the testing side and @nerolation has been implementing most of the specs for it. It would make sense to me though to get these changes here dialed in and approved and then rebase our changes off of these to use the same tracker? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a working implementation for BALs. I think we should consolidate the ideas from here and in the BALs PR. I'm not sure what the differences in use / design are between the implementations but it would be worth not duplicating work and syncing on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the BALs work is a superset of what we need -- I can setup a group with Me + Ignacio + Toni + other STEEL members so we can dial this in
No description provided.